Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support and CI workflow for riscv64 #1627

Merged
merged 5 commits into from
Sep 30, 2023

Conversation

leso-kn
Copy link
Contributor

@leso-kn leso-kn commented Aug 19, 2023

As there's been few recent activity on related PRs, this is #1574 rebased on latest main with:

  • git commit-attribution to involved developers and
  • sanetized commit messages to match the naming used by ring

Tested and working on Alpine Linux (riscv64 with musl libc).

Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this. I appreciate the thoroughness of this PR. A few questions.

build.rs Outdated Show resolved Hide resolved
include/ring-core/base.h Outdated Show resolved Hide resolved
include/ring-core/base.h Outdated Show resolved Hide resolved
mk/cargo.sh Outdated Show resolved Hide resolved
src/aead/aes.rs Outdated Show resolved Hide resolved
@briansmith
Copy link
Owner

There is a merge conflict in ci.yml that needs to be resolved before I can submit this to Github Actions. Please rebase this onto main.

@leso-kn
Copy link
Contributor Author

leso-kn commented Sep 19, 2023

Thanks for the thorough review :) Applied the pointed out suggestions and rebased onto main!

@briansmith
Copy link
Owner

Have you really tested this on riscv32? I'm wondering if we should just drop that part.

@leso-kn
Copy link
Contributor Author

leso-kn commented Sep 19, 2023

I personally only tested riscv64. The commit contains both as it's based on the work of @amock and @xfbs, while the second commit is based on @light4's implementation, which was targeted for riscv64 only.

I believe we will have tests through the CI workflow for riscv64, do you prefer that we add an entry in the CI config for riscv32 as well or rather to remove that part from the first commit?

@briansmith
Copy link
Owner

I believe we will have tests through the CI workflow for riscv64, do you prefer that we add an entry in the CI config for riscv32 as well or rather to remove that part from the first commit?

I'd rather we just remove it for now. We can add it back if/when we have somebody who is really needing it and who can help test it.

@leso-kn leso-kn force-pushed the riscv64-support branch 2 times, most recently from f2ad112 to 10af6ca Compare September 19, 2023 16:39
@leso-kn
Copy link
Contributor Author

leso-kn commented Sep 19, 2023

Alright then. I'll keep riscv32 support on this branch and limited this branch to riscv64 now (3bc2da6).

Regarding the previous comment, I've also added the missing use_clang in install-build-tools.sh.

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Merging #1627 (16837d1) into main (bb64b55) will increase coverage by 3.88%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1627      +/-   ##
==========================================
+ Coverage   92.12%   96.01%   +3.88%     
==========================================
  Files         132      133       +1     
  Lines       18917    19885     +968     
  Branches      199      199              
==========================================
+ Hits        17428    19092    +1664     
+ Misses       1451      754     -697     
- Partials       38       39       +1     

see 11 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -89,6 +89,9 @@
#elif defined(__MIPSEL__) && defined(__LP64__)
#define OPENSSL_64_BIT
#define OPENSSL_MIPS64
#elif defined(__riscv) && __riscv_xlen == 64
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BoringSSL uses __SIZEOF_POINTER__. I don't think that using the pointer size is as relevant for ring as it is for BoringSSL so I am open to using __riscv_xlen instead. Any thoughts on this?

#elif defined(__riscv) && __SIZEOF_POINTER__ == 8
#define OPENSSL_64_BIT
#define OPENSSL_RISCV64

@briansmith
Copy link
Owner

@leso-kn You probably didn't notice but the CI failed. It seems like the linking configuration isn't correct.

I would stil llike to use the clang-based build system, but if you can't get it working, I'm OK with reverting back to what you had before with the GCC-based build system.

@briansmith
Copy link
Owner

@leso-kn You probably didn't notice but the CI failed. It seems like the linking configuration isn't correct.

I would stil llike to use the clang-based build system, but if you can't get it working, I'm OK with reverting back to what you had before with the GCC-based build system.

It seems like you just need to change the linking so that it continues to be done by the GCC. See #1297 and #1057 where we had to do the same thing.

@briansmith
Copy link
Owner

PR #1663 is moving the stuff in base.h to target.h. I will merge PR #1663 ASAP so you can rebase this on top of main for the release. I am hoping to finish the BoringSSL merge today so there shouldn't be any new conflicts.

@briansmith
Copy link
Owner

I posted PR #1669 which rebases your changes on top of the current main branch.

@briansmith
Copy link
Owner

Thank you for your help with this!

@leso-kn
Copy link
Contributor Author

leso-kn commented Sep 30, 2023

Hey :) Sorry for the late reply, it was a quite busy week, TL is on vacation and the customer requested a lot of things at work.

Thanks for rebasing it! I'll do it here as well just for completedness, but feel free to merge either request – probably yours as the pipeline is already in progress I see.

@leso-kn leso-kn force-pushed the riscv64-support branch 4 times, most recently from 2da9d6d to 7d734f7 Compare September 30, 2023 09:47
@leso-kn
Copy link
Contributor Author

leso-kn commented Sep 30, 2023

@briansmith Rebasing done, with commit signing and original committer dates in case you prefer that. But feel free to merge either of the PRs :)

@leso-kn
Copy link
Contributor Author

leso-kn commented Sep 30, 2023

To see the difference between the PRs see here and here.

Content wise they're the same (git diff leso-kn/riscv64-support b/risc-v is equal).

@briansmith
Copy link
Owner

@briansmith Rebasing done, with commit signing and original committer dates in case you prefer that. But feel free to merge either of the PRs :)

OK, If you prefer this way, I will let CI run your PR and then merge yours!

leso-kn and others added 5 commits September 30, 2023 09:53
Co-authored-by: Alan Mock <alan@alanmock.com>
Co-authored-by: Patrick Elsen <pelsen@xfbs.net>
Signed-off-by: leso-kn <info@lesosoftware.com>
Co-authored-by: light4 <root@i01.io>
Signed-off-by: leso-kn <info@lesosoftware.com>
@@ -507,6 +512,9 @@ jobs:
- target: i686-unknown-linux-gnu
host_os: ubuntu-22.04

- target: riscv64gc-unknown-linux-gnu
host_os: ubuntu-22.04
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous CI run passed except coverage. coverage failed because this was missing. I added it and now coverage is passing.

@briansmith
Copy link
Owner

Thank you for all your assistance with this!

@briansmith briansmith merged commit 165e8a7 into briansmith:main Sep 30, 2023
266 checks passed
jsmolic added a commit to jsmolic/selenium that referenced this pull request Nov 23, 2023
Selenium-webdriver fails to compile on RISC-V due to outdated version of ring
used (https://bugs.gentoo.org/912999).
Ring has added RISC-V support in
briansmith/ring#1627, so update ring and
additional necessary crates to allow us to build on RISC-V. This change
was compile tested inside a virtual machine.
jsmolic added a commit to jsmolic/selenium that referenced this pull request Nov 23, 2023
Selenium-webdriver fails to compile on RISC-V due to outdated version of ring
used (https://bugs.gentoo.org/912999).
Ring has added RISC-V support in
briansmith/ring#1627, so update ring and
additional necessary crates to allow us to build on RISC-V. This change
was compile tested inside a virtual machine.
jsmolic added a commit to jsmolic/selenium that referenced this pull request Nov 23, 2023
Selenium-webdriver fails to compile on RISC-V due to outdated version of ring
used (https://bugs.gentoo.org/918098).
Ring has added RISC-V support in
briansmith/ring#1627, so update ring and
additional necessary crates to allow us to build on RISC-V. This change
was compile tested inside a virtual machine.
@ub-tech ub-tech mentioned this pull request Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants